-
Notifications
You must be signed in to change notification settings - Fork 59
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Clean dependencies #739
base: master
Are you sure you want to change the base?
Clean dependencies #739
Conversation
Very important PR. We have |
import JSON5 from 'json5'; | ||
import { lilconfig } from 'lilconfig'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is the tradeoff for this. is lilconfig feature matched with cosmiconfig (given what we and users might depend on)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Feature match is the idea of lilconfig
(but it has fewer dependencies and works faster). I moved from cosmiconfig
to lilconfig
in PostCSS Load Config and Size Limit without any problems.
@@ -1,7 +1,7 @@ | |||
import { NodePath } from '@babel/core'; | |||
import generate from '@babel/generator'; | |||
import * as t from '@babel/types'; | |||
import get from 'lodash/get'; | |||
import get from 'lodash.get'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really dislike these little packages. they may be smaller but it usually balloons the dep tree and number of transitive dependencies significantly. What is the actual cost saves between all of them over installing lodash. TBH I don't understand this sort of optimization, even on slow networks installing 70kb of text is not expensive, in the context of "putting files on disk to do development", I would imagine (unscientifically) having more limited deps, thereby reducing the amount of network hops and logic required by bundlers to install would be faster overall.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lodash
is 1.3 MB vs 23 KB for lodash.get
For instance, I don't have lodash
in subdependecies of my projects. Adding extra MB will slow down CI by a few seconds.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@RKatarine @jquense @ai can we replace lodash by native ECMAScript features?
This PR has been motivated by the issue
comsiconfig
tolilconfig
;lodash
with speciallodash.x
with a specific function;@types/fs-extra
globby
tofast-glob